Skip to content

Fix Play-ws stream() bug#1286

Merged
randomanderson merged 6 commits into
masterfrom
landerson/play-streaming
Mar 5, 2020
Merged

Fix Play-ws stream() bug#1286
randomanderson merged 6 commits into
masterfrom
landerson/play-streaming

Conversation

@randomanderson
Copy link
Copy Markdown
Contributor

@randomanderson randomanderson commented Mar 3, 2020 •
edited
Loading

This pull request fixes a bug in the play-ws instrumentation where calls to .stream() lead to a separate wrapper class being needed (#1252 ). Requiring these lines be added:

      if (asyncHandler instanceof StreamedAsyncHandler) {
        asyncHandler = new StreamedAsyncHandlerWrapper((StreamedAsyncHandler) asyncHandler, span);
      } else {
        asyncHandler = new AsyncHandlerWrapper(asyncHandler, span);
      }

Additionally, this pull request:

  • Refactors common code from the play-ws projects into a base project
  • Adds tests for the 4 ways play-ws can be used (java, java stream, scala, scala stream)
  • Adds a chunked encoding test to the HttpClientTest

Unfortunately, the tests also exposed a bug in the play-ws and/or scala promises instrumentation where callbacks to a promise aren't executed in the correct context. I added a flag to that HttpClientTest along with a //FIXME comment

@randomanderson randomanderson requested a review from a team as a code owner March 3, 2020 23:01
tylerbenson
tylerbenson approved these changes Mar 3, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@tylerbenson tylerbenson left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot going on here, but nothing stands out. I appreciate the additional testing.

... looks like there are some test failures to work through first though.

@randomanderson
Copy link
Copy Markdown
Contributor Author

randomanderson commented Mar 4, 2020

A lot going on here, but nothing stands out. I appreciate the additional testing.

... looks like there are some test failures to work through first though.

@tylerbenson It was a lot more straightforward before the refactor but the amount of copy/pasting was killing me. It probably should have been 2 separate PRs

@randomanderson randomanderson force-pushed the landerson/play-streaming branch from 3e51ecc to d6638f4 Compare March 4, 2020 15:42
@randomanderson randomanderson merged commit 0f65b4d into master Mar 5, 2020
@randomanderson randomanderson deleted the landerson/play-streaming branch March 5, 2020 15:42
@tylerbenson tylerbenson added this to the 0.46.0 milestone Mar 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@tylerbenson tylerbenson tylerbenson approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.46.0

Development

Successfully merging this pull request may close these issues.

2 participants

@randomanderson @tylerbenson